Skip to content

Enables oversampling for the Gecko IADC #95823

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Enables oversampling for the Gecko IADC #95823

wants to merge 1 commit into from
+10 −3

Conversation

wkhadgar
Copy link
Contributor

Enables the zephyr interpretation of oversampling for the IADC sequence, averaging the samples for up to 16 measurements.

@zephyrbot zephyrbot added area: ADC Analog-to-Digital Converter (ADC) platform: Silabs Silicon Labs labels Sep 10, 2025
@wkhadgar wkhadgar force-pushed the iadc-gecko-digital-average branch from 21fd75c to 260de3a Compare September 10, 2025 21:03
@Martinhoff-maker
Copy link
Member

@wkhadgar Hello, first, thanks for the feature update ! I will check but just to let you know that I've open a PR to rework the IADC driver with the scan feature. #94264. I will try to update my PR with the oversampling feature.

@wkhadgar
Copy link
Contributor Author

wkhadgar commented Sep 11, 2025 •
edited
Loading

Thanks @Martinhoff-maker!

I will try to update my PR with the oversampling feature.

I'm not sure if this is the best path. Considering the architecture overhead of single pull requests and the contributor guidelines, I consider it would be better for this small, contained and easily testable feature to be pushed to the upstream standalone first, and then your PR to be rebased on that change, avoiding implementing a new feature over the many already present in #94264. This would be better in general since revisions would be faster, stability easier to asset and the availability of the new feature sooner achieved!

@asmellby
Copy link
Contributor

asmellby commented Sep 12, 2025 •
edited
Loading

Thanks for the contribution! I have a couple of questions:

  • Did you consider using the analog oversampling feature of the hardware instead of (or in addition to) the digital averaging feature? Analog oversampling (up to 64x) combines with digital averaging (up to 16x) to increase the effective number of bits.
  • It looks like digital averaging is not available on xG21, so the IADC_DigitalAveraging_t type and digAvg member can't be used there. The xG21 support in Zephyr is incomplete, so the IADC node is actually missing from devicetree, which is why there are no CI errors. But it would be unfortunate to change the driver in such a way that it becomes incompatible. Would you consider either guarding it with an #ifdef or use analog oversampling instead?

Martinhoff-maker
Martinhoff-maker reviewed Sep 12, 2025
View reviewed changes
drivers/adc/iadc_gecko.c Outdated
initAllConfigs.configs[0].reference = channel_config->reference;

initAllConfigs.configs[0].digAvg = channel_config->digital_averaging;
init.warmup = iadcWarmupKeepWarm;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need to keep the IADC warm ? have you some delays if you do not activate this ? I'm not really in favor to keep the ADC warm by default because it will lead to a higher power consumption. It's about 5 us to warm the ADC before a conversion. On the other hand, there is nothing in the ADC API to give that kind of argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. Actually a personal take in here, was not planning on adding this to the PR itself, just slipped under my nose hahaha 😆

What you think of adding this as a DTS option? Or would be better to fully omit this and go for the defaults?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you really need it, please add it as a DT property like in the silabs_vdac driver. Otherwise, I suggest to go for the default value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really a must, and honestly I don't think it fits in this PR either, will remove it in favor of default value.

@wkhadgar
Copy link
Contributor Author

wkhadgar commented Sep 12, 2025 •
edited
Loading

  • Did you consider using the analog oversampling feature of the hardware

Actually did, just didn't though it to be the closest interpretation as other ADC drivers do, so I just kept with the standard "mean measure" analog of oversampling. Though it is very much appreciated to have the IADC oversampling take on this driver, being able to probe higher resolution is actually very useful in many contexts, I'm just not sure on how to approach this inside the ADC API itself. Wouldn't it be better as a separate DTS option (the higher resolution oversampling) and internally handled by the driver? In this case, I would be happy to take a look and open another PR implementing this as well.

  • But it would be unfortunate to change the driver in such a way that it becomes incompatible. Would you consider either guarding it with an #ifdef or use analog oversampling instead?

For the foremost argument above, the conditional compiling seems the nicer way to me.

@asmellby What are your thoughts?

@wkhadgar
drivers: adc: iadc_gecko: enables oversampling for the IADC
d6c610d 
Enables the zephyr interpretation of oversampling for the IADC
 sequence, averaging the samples for up to 16 measurements.

Signed-off-by: Paulo Santos <[email protected]>
@wkhadgar wkhadgar force-pushed the iadc-gecko-digital-average branch from 260de3a to d6c610d Compare September 16, 2025 22:02
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@Martinhoff-maker
Copy link
Member

@wkhadgar you can also use only the analog oversampling for the moment with sl_hal_iadc_config_osr_high_speed_t. It works the same way as the digital averaging and is compatible also with xg21 soc.

@Martinhoff-maker
Copy link
Member

@wkhadgar I implemented the oversampling in my PR with all the necessary for the xg21 compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Martinhoff-maker Martinhoff-maker Martinhoff-maker left review comments

@anangl anangl Awaiting requested review from anangl

@asmellby asmellby Awaiting requested review from asmellby

@jerome-pouiller jerome-pouiller Awaiting requested review from jerome-pouiller

@jhedberg jhedberg Awaiting requested review from jhedberg

At least 2 approving reviews are required to merge this pull request.

Assignees

@jhedberg jhedberg

@asmellby asmellby

Labels
area: ADC Analog-to-Digital Converter (ADC) platform: Silabs Silicon Labs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@wkhadgar @Martinhoff-maker @asmellby @jhedberg @zephyrbot